Support security group names in VPC Resource controller #389
Conversation
Add Security group name field to VPC resource controller
|
I have tested this, with some augmentation, on our cluster and it seems to work. We would need EKS to test this because we had to remove CreateNetworkInterfacePermission from the code (not something we can / need to do). |
GnatorX
changed the title
|
This would also be super useful for tying together security group policies generated by TF with a naming schema. Right now we use a cli to generate the sg pull the id out and then set it as a tf var for import and a variable for argocd. This would be a huge improvement in that workflow |
|
Thanks for the PR, I'm going to take a look this week. |
| // GroupNames contains the list of security group names that will be applied to the network interface of the pod matching the criteria. | ||
| type GroupNames struct { | ||
| // Groups is the list of EC2 Security Group Names that need to be applied to the ENI of a Pod. | ||
| // +kubebuilder:validation:MinItems=1 |
There was a problem hiding this comment.
I think we should have MinItems=0.
MinItems=1 would enforce that customers specify at least 1 SG name. What if they want to specify only SG IDs?
We need to enforce that both lists combined is not empty though.
There was a problem hiding this comment.
ya thats a good call out, let me see what we can do here
There was a problem hiding this comment.
The non-empty check I believe is checked on CreateNetworkInterface. Do you have different place in mind?
There was a problem hiding this comment.
Yeah, it would be good to include in CRD validation, let me see how to do this with kubebuilder.
| } | ||
|
|
||
| // For unit testing | ||
| func newEC2APIHelper(ec2Wrapper EC2Wrapper, clusterName, workerNodeVpcId string, |
There was a problem hiding this comment.
This is not required, you can use getMockWrapper in helper_test.go here and pass in any mock VPC ID.
There was a problem hiding this comment.
We added this here because need to to expose the internal cache for testing.
| } | ||
| sgNamesNotInCache := make([]string, 0) | ||
| for _, sgName := range securityGroupNames { | ||
| if sgId, ok := h.securityGroupNameToIdMap[sgName]; !ok { |
There was a problem hiding this comment.
I'm not sure caching is a good idea here.
Consider the scenario where a security group policy includes sg-example-name corresponding to sg-1111. This is added to securityGroupNameToIdMap, and some pod A is launched with sg-1111 attached to the branchENI. Now, customer deleted sg-1111 and created sg-2222 with the same name sg-example-name. But the map still points to old sg-1111 which no longer exists. So any pods launched after would be stuck in creation process as branchENI cannot be created(due to missing SG).
Need to evaluate if we always have to do an API call during pod creation or we can consider periodically refreshing the cache (TTL caching).
There was a problem hiding this comment.
I think a TTL caching make sense given (at least from our side) how rare we change delete security group and recreate with the same name. The cache was added to reduce the number of calls we hit EC2 API with.
| } | ||
|
|
||
| // GetSourceAcctAndArn constructs source acct and arn and return them for use | ||
| func GetSourceAcctAndArn(roleARN, region, clusterName string) (string, string, error) { |
There was a problem hiding this comment.
Any reason why this is moved from pkg/utils/helper.go?
There was a problem hiding this comment.
There is a circular dependency which required moving a few things out of utils such as this and the httpclient code
PR changes: Move to getsgforVPC, move SG names into SG, unique items …
|
Updated the PR. Let me know what you think about TTL cache. If you want to move in that direction. Do you have a recommendation of TTL cache, if not I will just pick up https://github.com/kubernetes/client-go/blob/v0.29.3/tools/cache/expiration_cache.go#L38 |
Thanks, will go through this. |
|
I am going to close this against #408 because it was simpler to rebase + squash |
Issue #, if available: #20
Description of changes:
Add support for Security group names
Credit: Most of the code was implemented by @abeer-stripe
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.